Skip to content

Use more standard form of release flag in MESON_ARGS#98

Closed
h-vetinari wants to merge 3 commits into
conda-forge:mainfrom
h-vetinari:meson
Closed

Use more standard form of release flag in MESON_ARGS#98
h-vetinari wants to merge 3 commits into
conda-forge:mainfrom
h-vetinari:meson

Conversation

@h-vetinari

@h-vetinari h-vetinari commented Sep 28, 2023

Copy link
Copy Markdown
Member

There are two different ways to set up a release flag to meson, --buildtype release, and -Dbuildtype=release. Apparently the latter is more common; at least it's the recommended way in the docs.

Since meson-python will by default add -Dbuildtype=release, the fact that the compiler-activation does the other kind leads to a duplicate specification and forces us to strip it out again as follows (e.g. in scipy, but affects all feedstocks using meson-python).

# meson-python already sets up a -Dbuildtype=release argument to meson, so
# we need to strip --buildtype out of MESON_ARGS or fail due to redundancy
MESON_ARGS_REDUCED="$(echo $MESON_ARGS | sed 's/--buildtype release //g')"

To avoid this, use the more common form of the flag.

PS. Stumbled over this old PR, where this was discussed with @rgommers
PPS. Sister PR of conda-forge/clang-compiler-activation-feedstock#112

@conda-forge-webservices

conda-forge-webservices Bot commented Sep 28, 2023

Copy link
Copy Markdown

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@h-vetinari h-vetinari mentioned this pull request Sep 28, 2023
Merged
@isuruf

isuruf commented Sep 28, 2023

Copy link
Copy Markdown
Member

This might break packages that does the opposite. So, it's not clear which one is better.

@h-vetinari

h-vetinari commented Sep 28, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

This might break packages that does the opposite.

That's clear, though I think it'd be good to decide on one and try to be consistent.

So, it's not clear which one is better.

Given that meson-python uses -Dbuildtype (not only the conda-forge version, but built-in upstream), and that the meson docs use this variant too, I think the balance tilts towards that.

@rgommers

rgommers commented Sep 28, 2023

Copy link
Copy Markdown

Given that meson-python uses -Dbuildtype (not only the conda-forge version, but built-in upstream), and that the meson docs use this variant too, I think the balance tilts towards that.

I agree. This change is known to avoid a bunch of argparsing complexity. It is right for all Python packages. In case there are non-Python packages that have an explicit --buildtype=release in their build.sh script in conda-forge, you'll get a clear error:

ERROR: Got argument buildtype as both -Dbuildtype and --buildtype. Pick one.

You'll fix that once, which is a simple 1-line diff, and does not require argument parsing.

meson-python isn't only relevant here because it's getting more popular, but because it's an intermediate layer that adds the incompatible flag. Such a layer does not exist for other languages, so there isn't the opposite problem if we make the switch here to using -Dbuildtype.

@h-vetinari h-vetinari mentioned this pull request May 2, 2024
Merged
@h-vetinari

h-vetinari commented Jul 11, 2024

Copy link
Copy Markdown
Member Author

I'm adapting the changes discussed on the clang side into #121

@h-vetinari h-vetinari mentioned this pull request Jul 11, 2024
Merged
@isuruf isuruf closed this in #121 Jul 16, 2024
@h-vetinari h-vetinari deleted the meson branch July 16, 2024 22:29
@h-vetinari h-vetinari mentioned this pull request Apr 16, 2025
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@beckermr beckermr Awaiting requested review from beckermr beckermr is a code owner
@isuruf isuruf Awaiting requested review from isuruf isuruf is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@h-vetinari @isuruf @rgommers